DM-44720: Support pickling of SlackException
#256
Conversation
Exception classes that call the `BaseException.__init__` method (possibly via `Exception`) with a different number of arguments than the `__init__` method of the derived exception class takes cannot be pickled and unpickled. Since arq uses pickle to record the exception of failed jobs, and we may want such exceptions to be derived from `SlackException` so that we can easily report them to Slack, this is an annoying limitation. The restriction is due to the properties of the default `__reduce__` method on `BaseException` and probably cannot be changed in Python. Work around this by using the pattern recommended at the end of the discussion in python/cpython#44791, namely avoid calling `BaseException.__init__` entirely. This means we can no longer rely on the default `__str__` behavior and must implement `__str__`. Add a test that `SlackException` and classes derived from it with different numbers of constructor arguments can be pickled and unpickled without loss of the information that goes into `to_slack`.
Some of the parameters in the test for the Google Storage mock were misspelled. This previously was not caught because that Python library wasn't typed, but the latest version has types and mypy started rejecting the misspelled arguments. Fix the spelling.
There was a problem hiding this comment.
Well this is quite interesting. I'll trust your research on this approach.
One thing I will note is that I hadn't considered sending Slack webhook messsages from the arq client rather than in the Arq worker function itself. In Noteburst, for instance, I catch any exceptions in the worker function exception itself and then send a Slack message in the exception handler before reraising the original exception (essentially doing my own version of SlackException). Noteburst also doesn't raise SlackExceptions from any code related to task services, so I guess that's why this hadn't come up. But in a codebase where everything is already a SlackException, I can see this being quite useful.
On the other hand, do you think it'd be useful for the arq workers to be sending the Slack messages rather than waiting for the client to consume the result?
|
I agree with that strategy, and largely that's what I'm doing, but the trick in my case is that the backend worker is running in a stack container and is supposed to have minimal dependencies. In order for it to send Slack messages directly, I would have to pull in Safir and and least httpx (and more than that if I didn't want to break Safir apart even further). Also, I want to do as little as possible in the backend worker since the developer has to supply that, and move as much shared code as possible to other places. The strategy I'm using is that the backend worker raises exceptions derived from Originally, I implemented this workaround entirely in |
Exception classes that call the
BaseException.__init__method (possibly viaException) with a different number of arguments than the__init__method of the derived exception class takes cannot be pickled and unpickled. Since arq uses pickle to record the exception of failed jobs, and we may want such exceptions to be derived fromSlackExceptionso that we can easily report them to Slack, this is an annoying limitation. The restriction is due to the properties of the default__reduce__method onBaseExceptionand probably cannot be changed in Python.Work around this by using the pattern recommended at the end of the discussion in python/cpython#44791, namely avoid calling
BaseException.__init__entirely. This means we can no longer rely on the default__str__behavior and must implement__str__.Add a test that
SlackExceptionand classes derived from it with different numbers of constructor arguments can be pickled and unpickled without loss of the information that goes intoto_slack.